feat(exec): drain guest output before attach#1022
Conversation
📦 BoxLite review — looks good ·
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExecution output now uses bounded channels and guest-side sequenced buffering. Dropped output is reported explicitly, WebSocket streaming is separated from completion reporting, and wait endpoints provide execution status across REST, runner, and CLI servers. ChangesExecution output and completion flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ExecProtocol
participant OutputManager
participant WebSocketAttach
participant WaitEndpoint
participant Execution
ExecProtocol->>OutputManager: deliver decoded output
WebSocketAttach->>OutputManager: attach to buffered stream
OutputManager-->>WebSocketAttach: ordered output or OutputDropped
WaitEndpoint->>Execution: await terminal completion
Execution-->>WaitEndpoint: execution status and exit code
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b9f4787 to
b0ee85c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/portal/interfaces/exec.rs (1)
338-388: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
route_output().awaitignoresshutdown_token— a stalled consumer blocks shutdown indefinitely.The
select!at lines 338-353 only guardsstream.message(); the per-messageSelf::route_output(...).awaitat line 358 (and the terminal-path flushes/sends at lines 371-384) run outside it. Sincempsc::Sender::send().awaiton the now-bounded channel blocks until capacity frees up, a consumer that is alive but not polling (hasn't dropped itsExecStdout/ExecStderr) makes this task ignoreshutdown_token.cancelled()for as long as the channel stays full — exactly the scenario the non-blockingtry_flush()on the cancellation branch was designed to avoid.🔧 Race the per-message routing against cancellation too
match output.transpose() { Some(Ok(output)) => { message_count += 1; - Self::route_output(output, &mut stdout, &mut stderr).await; + tokio::select! { + biased; + _ = shutdown_token.cancelled() => { + stdout.try_flush(); + stderr.try_flush(); + break; + } + _ = Self::route_output(output, &mut stdout, &mut stderr) => {} + } }The same gap applies to the error/EOF terminal flushes (lines 371-384), though its impact there is smaller since the loop is about to exit anyway.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/portal/interfaces/exec.rs` around lines 338 - 388, Update the attach-stream loop around route_output and the terminal error/EOF handling so every potentially blocking flush or send is raced against shutdown_token.cancelled(). Ensure cancellation exits promptly without awaiting a bounded-channel operation indefinitely, while preserving normal output ordering and existing cleanup behavior when no shutdown occurs.
🧹 Nitpick comments (1)
src/guest/src/service/exec/output.rs (1)
16-19: 🧹 Nitpick | 🔵 TrivialUnbounded ring accumulation across completed executions.
Each
OutputManagerretains its 1 MiB ring after the process exits (held byInner.outputand any live attach-stream clone), so total guest memory grows with the number of completed-but-uncleaned executions. The PR already notes TTL/cleanup as future work; consider bounding this with an eviction/TTL on completed executions before it becomes a guest-OOM vector under high exec churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guest/src/service/exec/output.rs` around lines 16 - 19, Bound completed execution state retained by OutputManager so its 1 MiB ring is not kept indefinitely after process exit. Add eviction or TTL cleanup for completed executions, including releasing Inner.output and any attach-stream-held clones when cleanup occurs, while preserving access for active executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/boxlite/src/portal/interfaces/exec.rs`:
- Around line 338-388: Update the attach-stream loop around route_output and the
terminal error/EOF handling so every potentially blocking flush or send is raced
against shutdown_token.cancelled(). Ensure cancellation exits promptly without
awaiting a bounded-channel operation indefinitely, while preserving normal
output ordering and existing cleanup behavior when no shutdown occurs.
---
Nitpick comments:
In `@src/guest/src/service/exec/output.rs`:
- Around line 16-19: Bound completed execution state retained by OutputManager
so its 1 MiB ring is not kept indefinitely after process exit. Add eviction or
TTL cleanup for completed executions, including releasing Inner.output and any
attach-stream-held clones when cleanup occurs, while preserving access for
active executions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 104bcb7e-857c-4403-a78a-aac86bcf1d0f
📒 Files selected for processing (9)
src/boxlite/src/litebox/exec.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/portal/interfaces/exec.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/tests/run_main_command.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/output.rssrc/guest/src/service/exec/state.rssrc/shared/proto/boxlite/v1/service.proto
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e8b966028
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/src/commands/serve/handlers/executions.rs (1)
110-140: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
wait/status responses never surfaceerror_message, unlike the REST server and Go runner.
execution_status_bodyonly emitsexecution_id,status, andexit_code. This isn't just a JSON-construction gap —ActiveExecution's wait task (mod.rs) only storesresult.exit_codefromExecution::wait(), discardingresult.error_messageentirely, so there's nothing to surface even if this function were extended. Both the Go runner (ExecutionInfoResponse.ErrorMessage) and the REST client'sExecutionStatusResponse.error_messagecarry this field — e.g. the container-init-death diagnosis attached when a process is SIGKILLed via PID-namespace teardown. Theboxlite servebackend is now the one implementation of this shared "wait" contract that can't report it.Consider storing
error_messagealongsideexit_codeonActiveExecution(populated from the samewait()call) and including it inexecution_status_bodywhen present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/src/commands/serve/handlers/executions.rs` around lines 110 - 140, Extend ActiveExecution to retain result.error_message alongside the exit code when its wait task processes Execution::wait(). Update execution_status_body and the related wait/status response path to include error_message when present, preserving the existing execution_id, status, and exit_code fields and omitting the new field when absent.
🧹 Nitpick comments (2)
src/boxlite/src/rest/litebox.rs (1)
73-90: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftSpawned WS pump task has no lifecycle handle; bounded output channel can block it forever.
tokio::spawn(...)return values are discarded in bothwire_attachandexec(). Combined with the new boundedstdout_tx/stderr_txchannels,attach_ws_pump'sstdout_tx.send(text).await(line 802/806) blocks the entire pump loop — including reading the eventual exit frame and sending keepalive pings — whenever the channel is full. The added testwait_returns_when_unread_output_fills_the_websocket_queuedemonstrates exactly this and has toattach.abort()manually to clean up; production callers have no equivalent hook. A caller that keeps anExecutionalive (e.g., just to callwait()/signal()) without drainingstdout()/stderr()to completion will leave the WS connection and task stuck open until theExecutionitself is dropped.Worth confirming this is the intended tradeoff (bounded memory vs. potentially long-lived stuck connections for non-draining consumers), and if so, consider retaining the
JoinHandleso it can be aborted oncewait_executionresolves (or documenting the drain requirement onExecution).Also applies to: 154-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/rest/litebox.rs` around lines 73 - 90, Retain the JoinHandle returned by the spawned attach_ws_pump task in both wire_attach and exec, and tie its lifecycle to wait_execution so the pump is aborted once execution completion resolves, including when output channels are not drained. Ensure the resulting Execution retains or exposes the necessary handle and cleanup occurs without changing normal output consumption behavior.apps/api/src/boxlite-rest/boxlite-proxy.controller.ts (1)
131-151: 🩺 Stability & Availability | 🔵 TrivialConfirm nothing upstream cuts this connection before the 25h
proxyTimeout.
proxyTimeoutonly bounds the proxy→runner leg. If the Node http server (main.ts, not in this changeset) or any load balancer/reverse proxy in front of this service has a shorter idle/socket timeout, a long-runningwaitcould still be truncated well before 25h, which would surface to SDK callers as a spurious network failure on the very endpoint meant to be authoritative for completion.Also applies to: 255-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts` around lines 131 - 151, The long-running proxyExecWait request may be terminated by shorter upstream Node HTTP server or load-balancer idle/socket timeouts despite its 25-hour proxyTimeout. Inspect and update the relevant server and deployment proxy timeout configuration so the connection remains valid for at least the full wait duration, covering the equivalent configuration for the other affected endpoint as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/rest/litebox.rs`:
- Around line 974-1015: Update wait_execution to retry transient transport
errors from get_with_timeout using bounded backoff consistent with
WS_RECONNECT_BUDGET before producing an ExecResult failure. Preserve immediate
handling of terminal statuses and non-terminal responses, and send the synthetic
error result only after the retry budget is exhausted.
---
Outside diff comments:
In `@src/cli/src/commands/serve/handlers/executions.rs`:
- Around line 110-140: Extend ActiveExecution to retain result.error_message
alongside the exit code when its wait task processes Execution::wait(). Update
execution_status_body and the related wait/status response path to include
error_message when present, preserving the existing execution_id, status, and
exit_code fields and omitting the new field when absent.
---
Nitpick comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.ts`:
- Around line 131-151: The long-running proxyExecWait request may be terminated
by shorter upstream Node HTTP server or load-balancer idle/socket timeouts
despite its 25-hour proxyTimeout. Inspect and update the relevant server and
deployment proxy timeout configuration so the connection remains valid for at
least the full wait duration, covering the equivalent configuration for the
other affected endpoint as well.
In `@src/boxlite/src/rest/litebox.rs`:
- Around line 73-90: Retain the JoinHandle returned by the spawned
attach_ws_pump task in both wire_attach and exec, and tie its lifecycle to
wait_execution so the pump is aborted once execution completion resolves,
including when output channels are not drained. Ensure the resulting Execution
retains or exposes the necessary handle and cleanup occurs without changing
normal output consumption behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a936e2d1-b314-4d66-b16f-c402e6108a55
📒 Files selected for processing (11)
apps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/runner/pkg/api/controllers/boxlite_exec.goapps/runner/pkg/api/controllers/boxlite_exec_test.goapps/runner/pkg/api/server.goopenapi/box.openapi.yamlsrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/types.rssrc/cli/src/commands/serve/README.mdsrc/cli/src/commands/serve/handlers/executions.rssrc/cli/src/commands/serve/mod.rs
|
Review follow-up:
Tests include REST wait retry coverage and the runner wait-timeout controller test. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/box.openapi.yaml`:
- Around line 644-647: Update the endpoint’s 200-response description in the
OpenAPI definition to document both outcomes: a terminal execution result and a
non-terminal status: "running" response when the 45-minute wait expires. Replace
the “Terminal execution result” wording while preserving the existing timeout
and retry behavior details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3709c682-a0e9-424e-a3a5-cc86ec7fa2b0
📒 Files selected for processing (7)
apps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/runner/pkg/api/controllers/boxlite_exec.goapps/runner/pkg/api/controllers/boxlite_exec_test.goopenapi/box.openapi.yamlsrc/boxlite/src/rest/litebox.rssrc/cli/src/commands/serve/README.mdsrc/cli/src/commands/serve/handlers/executions.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/boxlite-rest/boxlite-proxy.controller.ts
- src/cli/src/commands/serve/README.md
- src/cli/src/commands/serve/handlers/executions.rs
- src/boxlite/src/rest/litebox.rs
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28b16c618
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
BatmanByte
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b2ce1a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tracing::debug!(path = %path, error = %e, "WS attach: connect failed; output will not stream"); | ||
| return; |
There was a problem hiding this comment.
When the initial WebSocket attach fails (for example a transient proxy/runner upgrade failure), this task now only logs at debug and returns while the separately spawned /wait task can still report a clean exit code with no error_message. In that case stdout/stderr were never streamed, but callers of Execution::wait() cannot distinguish “command produced no output” from “we lost the output”; the previous fallback preserved the attach failure in the result. Please propagate this attach failure to the execution result/status path or otherwise surface it to the caller.
AGENTS.md reference: AGENTS.md:L69-L70
Useful? React with 👍 / 👎.
DorianZheng
left a comment
DorianZheng
left a comment
There was a problem hiding this comment.
most of these trace to two changes in this PR — the unbounded→bounded output channels, and the narrowed wait-retry predicate:
- one task routes both streams through blocking bounded sends, so reading only one of stdout/stderr can stall the other (and on the ws pump a full channel also blocks stdin + the keepalive ping).
wait_executionretries onlyNetwork, so a transient 503/timeout/429 becomes a cachedexit_code:-1.
the rest are resource retention (guest ring + leaked wait tasks/timers), the auto-pause vs long-poll interaction, and a few decoder/observability items.
| } | ||
|
|
||
| fn route_output(output: ExecOutput, stdout: &mut DecodedStream, stderr: &mut DecodedStream) { | ||
| async fn route_output( |
There was a problem hiding this comment.
a single task routes both stdout and stderr through the bounded(1024) channels here, and send_text awaits the send. a caller that reads only one stream (the pattern shown in the exec_handle docs) fills the other channel; route_output then parks on send().await, the task stops polling the guest stream, and the stream that is being read never advances. meanwhile the guest's 1 MiB ring overflows and drops output. the pre-change unbounded senders couldn't block here.
| 0x01 => { | ||
| tracing::trace!(len = text.len(), "WS attach: stdout frame"); | ||
| let _ = stdout_tx.send(text); | ||
| let _ = stdout_tx.send(text).await; |
There was a problem hiding this comment.
this send is now bounded and sits inside the single select! loop, so a full stdout channel blocks stdin forwarding and the keepalive ping too (same for the stderr send just below). an interactive exec whose consumer stops reading stdout — or an exec().wait() that never reads output — fills the 1024 slots, the pump parks on send().await, stdin stops flowing, no ping goes out, and the idle watchdog can reap a healthy connection.
| using_status_endpoint = true; | ||
| continue; | ||
| } | ||
| Err(error) => { |
There was a problem hiding this comment.
this terminal arm turns every non-Network, non-NotFound error into ExecResult{exit_code:-1}. an enveloped 503 (Portal), a timeout envelope (Internal), a bare 500 (Internal), and 429 (ResourceExhausted) all land here instead of retrying, and Execution::wait() caches the result via get_or_try_init — so a momentary gateway hiccup is permanently reported to the caller as the command having failed.
| } | ||
|
|
||
| @Get(':boxId/executions/:execId/wait') | ||
| async proxyExecWait( |
There was a problem hiding this comment.
this endpoint invites a wait-only client (no attach, no other calls), but proxyToRunner writes lastActivityAt once at request start. a 25-min command polled only through /wait has its activity stamped at t=0, so the auto-pause sweeper (900s) can pause the box mid-wait. with autoResume off the exec stays frozen; with it on the box flaps and time is lost.
| use tokio::sync::{watch, Mutex}; | ||
| use tonic::Status; | ||
|
|
||
| const BUFFER_CAPACITY_BYTES: usize = 1024 * 1024; |
There was a problem hiding this comment.
every ExecutionState now retains an OutputManager ring up to this 1 MiB cap, and the guest ExecutionRegistry has no remove/evict path, so nothing frees it after the exec exits. a box that runs many output-heavy commands over its lifetime pins ~1 MiB each until it stops — guest RSS grows monotonically toward OOM.
| }); | ||
| } | ||
|
|
||
| async fn drain<S>(&self, mut stream: S, source: OutputSource) |
There was a problem hiding this comment.
this drain loop (and the spawn helpers) emit no tracing — the previous forwarding tasks logged per-exec (execution=…, "…forwarding task ended"). a stuck or prematurely-ended exec can no longer be correlated to an exec_id or a specific stream from guest logs.
| stderr.send_bytes(chunk.data); | ||
| stderr.send_bytes(chunk.data).await; | ||
| } | ||
| Some(exec_output::Event::Dropped(_)) => { |
There was a problem hiding this comment.
the Dropped event is stream-agnostic and flushes both decoders. a loss on one stream then corrupts an in-flight split codepoint on the other: if stderr is mid-codepoint (holding e.g. 0xE2) when a stdout burst overflows the ring, stderr.flush() emits U+FFFD for the held byte and the surviving continuation bytes decode to more U+FFFD — a healthy ─ on stderr becomes replacement chars though stderr lost nothing.
| retry_backoff = retry_backoff.saturating_mul(2).min(WAIT_RETRY_BACKOFF_MAX); | ||
| delay | ||
| } | ||
| Err(BoxliteError::NotFound(_)) if !using_status_endpoint => { |
There was a problem hiding this comment.
using_status_endpoint latches true on the first /wait 404 and never resets, and a route-unsupported 404 is indistinguishable from a transient/real-exec 404 here. one early 404 (exec not yet visible on the addressed runner, or a proxy-level 404) permanently downgrades the client to immediate status polling for the whole exec, even against a server that supports the long-poll.
|
|
||
| /// Decode a wire chunk and forward any completed text to the receiver. | ||
| fn send_bytes(&mut self, data: Vec<u8>) { | ||
| async fn send_bytes(&mut self, data: Vec<u8>) { |
There was a problem hiding this comment.
on shutdown cancellation this path can lose output: send_bytes decodes and then awaits the send, so a cancel mid-send drops the already-decoded chunk; and try_flush only handles Closed, not Full, so a held partial's trailing U+FFFD is dropped when the channel is full. the normal EOF/error path awaits the flush and delivers these, so the shutdown path silently diverges.
| _ => { | ||
| // Server says the exec is still running — surface the | ||
| // synthesized cause so the caller sees the disconnect. | ||
| Err(BoxliteError::Network(error)) => { |
There was a problem hiding this comment.
Network errors are retried until WAIT_TOTAL_TIMEOUT (25h) with no consecutive-failure cap, so a clearly-persistent outage (connection refused / bare 502 / DNS failure all map to Network) makes Execution::wait() hang for up to a day instead of failing fast.
c7b2ce1 to
b68c092
Compare
BatmanByte
changed the title
b68c092 to
ed4679a
Compare
Start draining an execution's stdout and stderr as soon as its guest-side state is created. Retain a bounded one-megabyte replay buffer so commands are not blocked when no client attaches, and report overwritten output explicitly when a client attaches late. The dropped control event carries stdout and stderr byte counts so the host only resets the decoder that lost bytes. This keeps an intact stream from receiving a spurious replacement character.
ed4679a to
890ff64
Compare
2 participants
Summary
Drain each guest execution stdout and stderr as soon as it is created. This removes the dependency on a host Attach consumer for the guest process to make progress.
Guest stdio uses the Tokio I/O reactor rather than tokio fs. Idle pipes no longer occupy blocking-pool workers. Stdin uses the same reactor-backed path because PTY stdin and stdout descriptors share the non-blocking setting.
The guest retains at most 1 MiB of output for a late Attach. When that ring overwrites old bytes, Attach emits an OutputDropped control event with separate stdout and stderr byte counts. The host renders the notice and resets only the affected UTF-8 decoder.
Scope
This PR intentionally contains only guest exec stdio, the shared gRPC protocol, and the local gRPC consumer needed to render it. It does not change REST wait behavior, the runner, API, CLI, or host output queueing.
Verification
The integration commands ran outside the sandbox against real macOS Hypervisor VMs.
make fmt:check is currently blocked before Rust checking by 186 pre-existing Prettier violations in untouched generated apps client files; make fmt:check:rust passed.